refactor!: metrics #2464

Arqu · 2024-07-05T12:07:27Z

Description

Cleans up stale counters
Introduces unique daily node counts on relays
Introduces a metrics dumper so we can create dumps of metrics and also view them in doctor plot
Introduces new trace level counters

Breaking Changes

Introduces metrics_dump_path on the top level CLI parameters, which if set will make sure metrics are collected at regular intervals and written to the provided path in CSV format.

Notes & open questions

Change checklist

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

b5 · 2024-07-19T13:53:11Z

iroh-blobs/Cargo.toml

@@ -72,7 +72,7 @@ package = "iroh-quinn"
 version = "0.10"

 [features]
-default = ["fs-store"]
+default = ["fs-store", "metrics"]


so this means metrics collection is on by default in blobs now, or just that the feature is available?

Reworked so that dep is always available but metrics collection is not on by default.

b5 · 2024-07-19T13:54:10Z

iroh-dns-server/src/metrics.rs

@@ -9,7 +9,7 @@ use struct_iterable::Iterable;
 pub struct Metrics {
    pub pkarr_publish_update: Counter,
    pub pkarr_publish_noop: Counter,
-    pub pkarr_publish_error: Counter,
+    // pub pkarr_publish_error: Counter,


stray comment, plz remove

The point of this PR was for the team to claim which ones they would like to keep as they are unused. I'm going to nuke all the ones people didn't vote to keep.

b5 · 2024-07-19T13:55:41Z

iroh-net/src/magicsock/metrics.rs

-    pub rebind_calls: Counter,
+    // pub rebind_calls: Counter,
    pub re_stun_calls: Counter,
    pub update_endpoints: Counter,

    // Sends (data or disco)
-    pub send_relay_queued: Counter,
-    pub send_relay_error_chan: Counter,
-    pub send_relay_error_closed: Counter,
-    pub send_relay_error_queue: Counter,
+    // pub send_relay_queued: Counter,
+    // pub send_relay_error_chan: Counter,
+    // pub send_relay_error_closed: Counter,
+    // pub send_relay_error_queue: Counter,
    pub send_ipv4: Counter,
-    pub send_ipv4_error: Counter,
+    // pub send_ipv4_error: Counter,
    pub send_ipv6: Counter,
-    pub send_ipv6_error: Counter,
+    // pub send_ipv6_error: Counter,


remove commented-out code

b5 · 2024-07-19T13:55:58Z

iroh-net/src/magicsock/metrics.rs

@@ -40,7 +40,7 @@ pub struct Metrics {
    pub sent_disco_ping: Counter,
    pub sent_disco_pong: Counter,
    pub sent_disco_call_me_maybe: Counter,
-    pub recv_disco_bad_peer: Counter,
+    // pub recv_disco_bad_peer: Counter,


remove commented-out code

b5 · 2024-07-19T13:56:06Z

iroh-net/src/magicsock/metrics.rs

@@ -49,7 +49,7 @@ pub struct Metrics {
    pub recv_disco_ping: Counter,
    pub recv_disco_pong: Counter,
    pub recv_disco_call_me_maybe: Counter,
-    pub recv_disco_call_me_maybe_bad_node: Counter,
+    // pub recv_disco_call_me_maybe_bad_node: Counter,


flub · 2024-07-19T14:27:21Z

iroh-dns-server/src/metrics.rs

@@ -9,7 +9,7 @@ use struct_iterable::Iterable;
 pub struct Metrics {
    pub pkarr_publish_update: Counter,
    pub pkarr_publish_noop: Counter,
-    pub pkarr_publish_error: Counter,
+    // pub pkarr_publish_error: Counter,


this is obviously guessing at usage. but i'm surprised you want to remove this. i'm used to seeing most counters in at least a pair of _total and _error so you can do error ratios.

I agree, however these are unused counters and I think those can be misleading as you expect to see something in errors yet they never increment even if there was an error.

flub · 2024-07-19T14:28:57Z

iroh-net/src/magicsock.rs

@@ -1762,25 +1762,30 @@ impl Actor {
        };

        loop {
+            inc!(Metrics, actor_tick_main);


all these _tick_ metrics are basically trace-level metrics. is this really a good idea? We have trace-level logging for this. What do you expect these to be used for?

we used these quite a lot in the past to detect the health of tokio loops

So 2 reasons for this:

In general I'd like to move away from trace logging and shift towards trace metrics. Much easier to reason with it on a chart. Though logs do have their place where explicit order is really important.

We actually used these in development MUCH more and we're more useful to us than "product level health" metrics which you look at when deployed.

I could follow up on this and maybe introduce a split for "trace_metrics" vs "metrics" if people find these too much.

The general idea is to use metrics in your dev workflow too.

## Description Extends several pieces: - introduces a "metric dumper" which is just a way of saying you can sample the internal metrics and write them to a CSV file (which should come in handy for 3 pieces that should come down the line; 1) CI using these to validate behavior, 2) debug dumps from 3rd parties, 3) local debugging) - along with the dumper the `doctor plot` has been extended to be able to read those dumps and also just generally improved some rough edges so it's less error prone. - node counts are here for relays. You now have a derived metric which simply counts unique daily node connections. Sample usage for the metrics dumper: `cargo run --bin iroh --all-features -- --metrics-dump-path test.metrics.csv start` Sample of the plotter: `cargo run --bin iroh --all-features -- doctor plot --timeframe 30 --interval 10 --file metrics.dump.csv magicsock_actor_tick_main_total,magicsock_actor_tick_msg_total,magicsock_actor_tick_endpoint_heartbeat_total,magicsock_actor_tick_endpoints_update_receiver_total,magicsock_actor_tick_re_stun_total` ## Breaking Changes  ## Notes & open questions This is merging into #2464 as part of the larger metrics refactor. ## Change checklist - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented.

## Description - Cleans up stale counters - Introduces unique daily node counts on relays - Introduces a metrics dumper so we can create dumps of metrics and also view them in doctor plot - Introduces new trace level counters ## Breaking Changes - Introduces `metrics_dump_path` on the top level CLI parameters, which if set will make sure metrics are collected at regular intervals and written to the provided path in CSV format. ## Notes & open questions  ## Change checklist - [ ] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented.

Arqu mentioned this pull request Jul 17, 2024

feat: metrics dumps & extended metrics #2519

Merged

4 tasks

Arqu self-assigned this Jul 19, 2024

Arqu added this to the v0.21.0 milestone Jul 19, 2024

Arqu marked this pull request as ready for review July 19, 2024 11:22

b5 reviewed Jul 19, 2024

View reviewed changes

flub reviewed Jul 19, 2024

View reviewed changes

Arqu force-pushed the arqu/fresh_counters branch from d94a592 to 53bb3bd Compare July 22, 2024 14:41

Arqu requested a review from dignifiedquire July 22, 2024 15:23

dignifiedquire approved these changes Jul 22, 2024

View reviewed changes

dignifiedquire enabled auto-merge July 22, 2024 15:55

dignifiedquire changed the title ~~refactor: metrics~~ refactor!: metrics Jul 22, 2024

Arqu added 9 commits July 22, 2024 18:43

refactor: metrics

79559c8

fix builds

e2cac05

more build fixes

972818f

cleanup

3cf985a

rebase cleanup

ef259dc

drop stale counters

d82dce5

fix builds

049a2c4

rebase cleanup

f4128fe

Arqu force-pushed the arqu/fresh_counters branch from a5db957 to f4128fe Compare July 22, 2024 16:50

older ratatui due to itertool clash

51aa474

dignifiedquire added this pull request to the merge queue Jul 22, 2024

Merged via the queue into main with commit 09e9746 Jul 22, 2024
25 of 26 checks passed

Arqu deleted the arqu/fresh_counters branch July 22, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: metrics #2464

refactor!: metrics #2464

Arqu commented Jul 5, 2024 •

edited

Loading

b5 Jul 19, 2024

Arqu Jul 22, 2024

b5 Jul 19, 2024

Arqu Jul 22, 2024

b5 Jul 19, 2024

b5 Jul 19, 2024

b5 Jul 19, 2024

flub Jul 19, 2024

Arqu Jul 22, 2024

flub Jul 19, 2024

dignifiedquire Jul 22, 2024

Arqu Jul 22, 2024

Arqu Jul 22, 2024

refactor!: metrics #2464

refactor!: metrics #2464

Conversation

Arqu commented Jul 5, 2024 • edited Loading

Description

Breaking Changes

Notes & open questions

Change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu commented Jul 5, 2024 •

edited

Loading